Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add doubly LinkedList implementation to expose to scripting #12

Merged
merged 25 commits into from
Sep 17, 2020
Merged

Conversation

Xrayez
Copy link
Contributor

@Xrayez Xrayez commented Sep 10, 2020

Closes godotengine/godot#7194.
Helps godot-extended-libraries/godot-ideas#11.

Introduces LinkedList and ListNode classes, which are mostly an instantiation of built-in List<Variant> template/datatype.

To-do


Godot proposal: godotengine/godot-proposals#1522.

@Xrayez Xrayez added the feature 💡 New feature proposal label Sep 10, 2020
A bit too verbose, and `LinkedList` is more common name in other languages.
Not to mention that `Array` already handles `Variant` types.
Automatically cleanup allocated data from elements, and the elements themselves.
@Xrayez Xrayez changed the title Add VariantList doubly linked list implementation to expose to scripting Add doubly LinkedList implementation to expose to scripting Sep 10, 2020
@Xrayez
Copy link
Contributor Author

Xrayez commented Sep 11, 2020

Tried to port existing List::swap, bug confirmed upstream: godotengine/godot#41981.

This took some time to get right (hopefully). The tests should
cover the swap functionality which is currently broken in Godot.
Had to rename underlying methods because they would conflict with
property names. In Godot 4.0, it would also fail the tests.

It's recommended to use properties in GDScript for this. In other langs,
you'd simply use corresponding linked list data structure built-in anyway.
For consistency with godotengine/godot#34028.

Note that `erase` was renamed to `remove` in Goost.
Removes the need for allocating array as in `Node.get_children()`.

```gdscript
var list := LinkedList.new()
list.push_back("Goost")
list.push_back(37)
list.push_back(Color.blue)
for element in list:
	print(element)
```
Turns out iterating over a list with a custom for loop iterator is
somewhat slower via script than doing the same thing by allocating
an array, but both options are acceptible.
The name is quite common as seen in various linked list
tutorials over the Internet. This may not particularly true
if you go into details of what's considered an item in a
particular collection, but this is just too picky to think about
this, the engine caters to beginner use cases so I don't
see a reason why this should be made an abstract concept,
especially since it's likely that no other similar data structure would
be ever implemented in Goost (but only time will tell!)
This should be available in Godot >=3.2.3 or so.
Not using the `override` keyword in this case, makes it backward compatible.
Supports creating list nodes from any `Variant` compatible datatype.
@Xrayez
Copy link
Contributor Author

Xrayez commented Sep 15, 2020

This is how icons look like:

goost_linked_list_and_node

@zwparchman
Copy link

Linked lists (both single and double) are almost always the wrong choice for performance. While it's true adding and removing is constant time that assumes you have a reference to the list node you want to insert after already.

Traversing a list to find where you want to insert an item is much more expensive than inserting into the middle of an array even though they both share algorithmic complexity. This is due to a lists terrible interaction with the processor cache. There are many benchmarks scattered around the internet showing just how slow std::list is compared to std::vector. If you are pushing and popping from the front often a circular buffer is a better data structure.

An article on the subject can be found at https://baptiste-wicht.com/posts/2012/12/cpp-benchmark-vector-list-deque.html

It's worth noting that the excessive traversal cost is also payed when retrieving data from the list so using one to store often used information is often not advisable. Not only is it slow to traverse but doing so evicts other things from the cache which can cause general program performance to drop.

Given the reason to use a Linked list over an array will almost always be based on algorithmic complexity I don't think that it is unreasonable to ask for a real world use case where a list based solution performs better than an array (or circular buffer) based solution.

@Xrayez
Copy link
Contributor Author

Xrayez commented Sep 16, 2020

Yeah I guess performance is not priority for implementing such a data structure anyway, but doing so in C++ vs GDScript already provides some performance boost, but only when comparing C++ vs GDScript performance, and not list vs vector/array.

This is mostly similar/closer to graph node traversal (a path graph could be seen a list, for instance), certainly not efficient but the use cases are generally different. It's possible to nest linked lists inside other nodes/elements with the current implementation as well, making this even more flexible.

You may want to keep a general discussion going at godotengine/godot-proposals#1522 as well. 🙂

@Xrayez
Copy link
Contributor Author

Xrayez commented Sep 17, 2020

I think the current changes are complete and sufficient, the LinkedList API closely resembles the Godot's List<T> implementation, documented all methods and covered with tests, even fixed some bugs in Godot's List itself, some additional features can be added if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature 💡 New feature proposal topic:core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose double linked list to script
2 participants